Fix nested form body parsing happening for all content types#305
Open
cllns wants to merge 6 commits into
Open
Conversation
Constructing `Rack::Request.new(env)` just to read `#media_type` ran on
every request — including GETs that have no body to parse — costing one
allocation per check (plus four more on POSTs once `#media_type` parses
the header). Read `env["CONTENT_TYPE"]` directly instead: exact match
for the bare type, `start_with?("...;")` for the params form.
This also lets the helper short-circuit before touching `rack.input`.
Previously, main called `input.rewind` unconditionally inside the body
parse guard; the new check skips that work whenever `CONTENT_TYPE` is
absent or non-form, so GETs no longer pay the rewind cost at all.
Measured locally: the form-urlencoded check is 3-6x faster across GET,
form POST, and JSON POST shapes, with zero allocations on GETs and bare
form POSTs (down from 1 and 5 respectively).
timriley
approved these changes
May 15, 2026
| content_type = env[CONTENT_TYPE] | ||
| return false unless content_type | ||
|
|
||
| content_type == FORM_URLENCODED_MEDIA_TYPE || |
Member
There was a problem hiding this comment.
Suggested change
| content_type == FORM_URLENCODED_MEDIA_TYPE || | |
| content_type.downcase! == FORM_URLENCODED_MEDIA_TYPE || |
Feels like we should do this for maximum compatibility, and preservation of our existing behaviour. Rack::Request#media_type returns a downcased string courtesy of its internal MediaType.type. And if we're aiming to follow the spec strictly, it does say that these values are case insensitive.
Comment on lines
+1069
to
+1070
| content_type == FORM_URLENCODED_MEDIA_TYPE || | ||
| content_type.start_with?("#{FORM_URLENCODED_MEDIA_TYPE};") |
Member
There was a problem hiding this comment.
Suggested change
| content_type == FORM_URLENCODED_MEDIA_TYPE || | |
| content_type.start_with?("#{FORM_URLENCODED_MEDIA_TYPE};") | |
| content_type.downcase!.start_with?(FORM_URLENCODED_MEDIA_TYPE) |
In fact, I wonder whether we could actually just simplify it to this?
It'd work fine for both type/subtype as well as type/subtype;parameter=value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have a JSON API Hanami app and when benchmarking it, I had some generated code that POSTed with "%" in a field, as in "Improves speeds by 50%". This surprisingly raised a
Rack::QueryParser::InvalidParameterError.This is because we run
Rack::Utils.parse_nested_queryon the request body for every request, regardless of Content-Type. This causes JSON bodies to leak into router.params (as evidenced by the JSON regression test failure added here:{from: "ui", "{\"foo\":\"bar\"}": nil}), while bodies containing a literal%(#237) or multipart binary bytes (#296, other regression test) raiseRack::QueryParser::InvalidParameterErrorbecause it's being parsed as form input (nested query) and the % is interpreted as malformed URL encoding.#237 was closed with #240 as the workaround, but that was at a different time in the frameworks history: when we required JSON parsing to be done via a body parser in this repo. Now we're more flexible and parse it later on, so the problem gets triggered upstream. This fixes it at its proper place, only using
::Rack::Utils.parse_nested_queryfor forms.Closes #296